-
Notifications
You must be signed in to change notification settings - Fork 27
New session opening mechanism for StorageNodes #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I'm also going to detect whether old method is applied for the sessions with opened connection and throw an exception telling that this is not supported. |
Orm/Xtensive.Orm.Tests/Issues/IssueJira0614_TypeMappingCachingInMaterializationContext.cs
Outdated
Show resolved
Hide resolved
| using (var session = domain.OpenSession()) { | ||
| session.SelectStorageNode(WellKnown.DefaultNodeId); | ||
| _ = domain.StorageNodeManager.AddNode(nodeConfiguration); | ||
| var selectedNode = domain.SelectStorageNode(WellKnown.DefaultNodeId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For cases like this, we would introduce GetOrAddNode method (see below).
var storageNode = domain.StorageNodeManager.GetOrAddNode(nodeConfiguration);
using (var session = storageNode.OpenSession()) {
...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you build nodes on demand and use them just after build like in the example you showed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we do. On application start, we don't have any nodes added. On the first request, we build a domain and then add the corresponding node. All subsequent requests either use the existing node or add a new one on the first request to that node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The disadvantage of the suggested approach is that GetOrAddNode method takes node configuration as a parameter but the existing GetNode method gets just node id. I think we would exclude node id from config and pass it separately. So the sample would transform as follows.
var storageNode = domain.StorageNodeManager.GetOrAddNode(nodeId, nodeConfiguration);
using (var session = storageNode.OpenSession()) {
...
}
// AddNode method should also be changed for consistency.
_ = domain.StorageNodeManager.AddNode(nodeId, nodeConfiguration);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NodeConfiguration is required to have nodeId anyway, except for default node. It validates before StorageNode build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it is used only in the validation. The Validate method is called by the internal BuildNode method. In turn, the BuildNode method is called only from the AddNode.
As you can see we can easily transform all this chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I added extra check for nodeId to GetOrAdd. Take a look at new new commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep it this way, or even remove the GetOrAddNode method for now?
I'd suggest merging this PR to the 6.0 branch and to the master then.
Once the code is in the master I'd think about further improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will keep StorageNodeManager API the same. GetOrAdd functionality can be implemented as an extension in your solution. Later we could integrate your implementation to DataObjects.Net
Instead of opening a session and then select
StorageNodefor it we select a node and then open session for the selected node. It allows asynchronous session opening to work fine.Old method of storage node selection is marked
[Obsolete]but works for synchronously opened sessions.